refact: nydusd's state machine #439

ccx1024cc · 2022-05-18T12:50:28Z

QUESTION

UPGRADING and INTERRUPTED are designed for hot-upgrading, which is a use case instead of daemon's state. As a result, they are weird in lack of upgrading function.

DESIGN

Merge UPGRADING and INTERRUPTED to READY. READY means service is well-configured, but waiting for trigger. For fuse-based daemon, it means fuse device is mounted, while fuse mountpoint may be not attached. The daemon lives in INIT -> READY -> RUNNING -> READY -> DIE.

CHANGE

Daemon context with new status.
Async TerminateFuseService, Umount actions.
Start endpoint, which is usually used after takeover endpoint.
Async exit after fuse device unmounted abnormally.

imeoer · 2022-05-19T04:13:56Z

api/src/http_endpoint_v1.rs

+        kicker: &dyn Fn(ApiRequest) -> ApiResponse,
+    ) -> HttpResult {
+        match (req.method(), req.body.as_ref()) {
+            (Method::Put, None) => {


PUT method in RESTful API design means that the operation can be executed multiple times and always have an idempotent state, is this what the API expects?

PUT method in RESTful API design means that the operation can be executed multiple times and always have an idempotent state, is this what the API expects?

It's expected that daemon in READY is trigger to RUNNING. Just like takeover endpoint with PUT method, it's required for daemon in certain status.

changweige · 2022-05-19T09:28:56Z

src/bin/nydusd/daemon.rs

+        }
+
+        if s == DaemonState::RUNNING {
+            self.on_event(DaemonStateMachineInput::Stop)?;


Why not merge the two events handling if blocks.
They both react on Stop event

Why not merge the two events handling if blocks. They both react on Stop event

The INIT can transfer to DIE by stop directly. While the RUNNING is transferred to DIE via READY. For INIT, there is only one stop event, instead of two stop events for RUNNING.

src/bin/nydusd/fusedev.rs

changweige · 2022-05-23T02:21:15Z

Please check the patch's changelog, it is somewhat a mess of duplicated messages. Can you squash the SOB lines?
I suggest adding this PR's description into the commit message.

ccx1024cc · 2022-05-23T02:27:34Z

Please check the patch's changelog, it is somewhat a mess of duplicated messages. Can you squash the SOB lines? I suggest adding this PR's description into the commit message.

Good idea! 3q

changweige · 2022-05-23T02:36:27Z

api/src/http.rs

@@ -289,6 +291,7 @@ lazy_static! {
        r.routes.insert(endpoint_v1!("/daemon/events"), Box::new(EventsHandler{}));
        r.routes.insert(endpoint_v1!("/daemon/backend"), Box::new(FsBackendInfo{}));
        r.routes.insert(endpoint_v1!("/daemon/exit"), Box::new(ExitHandler{}));
+        r.routes.insert(endpoint_v1!("/daemon/start"), Box::new(StartHandler{}));


Shall we relocate the API path to /daemon/fuse/start since the handler of endpoint /daemon/start just starts fuse server's service loop. For nydusd itself, it is already started, otherwise how it can handle API requests?

Shall we relocate the API path to /daemon/fuse/start since the handler of endpoint /daemon/start just starts fuse server's service loop. For nydusd itself, it is already started, otherwise how it can handle API requests?

The start endpoint works with status Ready and RUNNING, which are designed for only fuse but also for other implements. Maybe run is more comfortable?

src/bin/nydusd/fusedev.rs

changweige · 2022-05-23T02:42:56Z

src/bin/nydusd/daemon.rs

-        Mount => Running [StartService],
-        Takeover => Upgrading [Restore],
-        Exit => Die[StopStateMachine],
+        Mount => Ready,


Seems we no longer need Mount event since nothing is done when migrating state to Ready

Seems we no longer need Mount event since nothing is done when migrating state to Ready

In fact, we shall do Mount action. However, it's a bit risky now. Because the Mount content are different in different cases, such as upgrade startup, normal startup, endpoint startup. In total, You're right. There will be abstraction for Mount action in future.

changweige · 2022-05-23T03:30:19Z

src/bin/nydusd/fusedev.rs

+                .map_err(|e| {
+                    DaemonError::WaitDaemon(
+                        *e.downcast::<Error>()
+                            .unwrap_or_else(|e| Box::new(eother!(e))),


Um. Hard error conversion path to io::Error, maybe let WaitDaemon wrap another type of error message is an easier way. But has nothing to do with this PR.

src/bin/nydusd/fusedev.rs

changweige · 2022-05-23T03:47:53Z

src/bin/nydusd/fusedev.rs

@@ -477,12 +508,13 @@ pub fn create_nydus_daemon(
        inflight_ops: Mutex::new(Vec::new()),
        result_receiver: Mutex::new(result_receiver),
        trigger: Arc::new(Mutex::new(trigger)),
-        threads: Mutex::new(Vec::new()),
+        state_machine_thread: Mutex::new(None),
+        fuse_service_threads: Mutex::new(Vec::new()),


Why is reaping fuse threads important?
Only wait for state machine termination?

Why is reaping fuse threads important? Only wait for state machine termination?

Yep, seeking for waiting state machine and fuse service thread termination separately. Because daemon status is only updated after successful action. For example, interrupt action is executed, then wait fuse service workers done, finally the daemon status is changed from RUNNING to READY.

bergwolf

LGTM!

`UPGRADING` and `INTERRUPTED` are designed for hot-upgrading, which is a use case instead of daemon's state. As a result, they are weird in lack of upgrading function. Merge `UPGRADING` and `INTERRUPTED` to `READY`. `READY` means service is well-configured, but waiting for trigger. For fuse-based daemon, it means fuse device is mounted, while fuse mountpoint may be not attached. The daemon lives in `INIT` -> `READY` -> `RUNNING` -> `READY` -> `DIE`. 1. Daemon context with new status. 2. Async `TerminateFuseService`, `Umount` actions. 3. `Start` endpoint, which is usually used after `takeover` endpoint. 4. Async exit after fuse device unmounted abnormally. Signed-off-by: 泰友 <cuichengxu.ccx@antgroup.com>

ccx1024cc force-pushed the upmaster branch from 96d976d to d48dcb3 Compare May 18, 2022 12:52

ccx1024cc requested review from bergwolf, jiangliu, imeoer and liubogithub May 18, 2022 12:58

imeoer reviewed May 19, 2022

View reviewed changes

changweige reviewed May 19, 2022

View reviewed changes

src/bin/nydusd/fusedev.rs Outdated Show resolved Hide resolved

ccx1024cc requested review from imeoer and changweige May 20, 2022 03:07

ccx1024cc force-pushed the upmaster branch 2 times, most recently from 0cbb1eb to 774c6c0 Compare May 23, 2022 02:29

changweige reviewed May 23, 2022

View reviewed changes

ccx1024cc force-pushed the upmaster branch from 774c6c0 to da31b99 Compare May 23, 2022 10:33

bergwolf approved these changes May 30, 2022

View reviewed changes

ccx1024cc force-pushed the upmaster branch from da31b99 to 8a7b4ce Compare May 31, 2022 06:30

ccx1024cc requested review from bergwolf and changweige May 31, 2022 06:50

imeoer merged commit 6be13a7 into dragonflyoss:master May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refact: nydusd's state machine #439

refact: nydusd's state machine #439

ccx1024cc commented May 18, 2022

imeoer May 19, 2022

ccx1024cc May 19, 2022

changweige May 19, 2022

ccx1024cc May 20, 2022

changweige commented May 23, 2022 •

edited

Loading

ccx1024cc commented May 23, 2022

changweige May 23, 2022

ccx1024cc May 23, 2022

changweige May 23, 2022

ccx1024cc May 23, 2022

changweige May 23, 2022

changweige May 23, 2022

ccx1024cc May 23, 2022

bergwolf left a comment

refact: nydusd's state machine #439

refact: nydusd's state machine #439

Conversation

ccx1024cc commented May 18, 2022

QUESTION

DESIGN

CHANGE

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changweige commented May 23, 2022 • edited Loading

ccx1024cc commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergwolf left a comment

Choose a reason for hiding this comment

changweige commented May 23, 2022 •

edited

Loading